-
Notifications
You must be signed in to change notification settings - Fork 543
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add a new descendants api as well as deeper element queries to ElementHandle #5433
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should visit_element be removed. It is a bit redundent since you can do HasElementHandle::root_element(component).visit_descendants
(the only difference is that it visits the root)
internal/core/item_tree.rs
Outdated
@@ -312,21 +313,30 @@ impl ItemRc { | |||
r.upgrade()?.parent_item() | |||
} | |||
|
|||
// FIXME: This should be nicer/done elsewhere? | |||
/// Returns true if this item is visible from the root of the item tree. Note that this will return | |||
/// false for `Clip` elements, even if they are "visible". |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will it? All Clip elements? Why is that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe I'm missing something (I tried to cover that also in the test I added in search_api.rs), but the old code did this:
pub fn is_visible(&self) -> bool {
let item = self.borrow();
let is_clipping = crate::item_rendering::is_clipping_item(item);
let geometry = self.geometry();
if is_clipping && (geometry.width() <= 0.01 as _ || geometry.height() <= 0.01 as _) {
return false;
}
if let Some(parent) = self.parent_item() {
parent.is_visible()
} else {
true
}
}
That suggests to me that is_clipping_item()
returns true for Clip
elements that are enabled for clipping (clip true) and the visible pass gives them the null width/height. So I think that'll return false.
The new code should return an empty clip rectangle from the call to absolute_clip_rect_and_geometry
because of this:
if crate::item_rendering::is_clipping_item(item) {
clip = geometry.intersection(&clip).unwrap_or_default();
}
And then the intersection of itself should be empty.
Should I add a direct unit test in here perhaps?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A test for that would be nice.
So if you have a
outer := Rectangle {
clip: true;
inner := Rectangle { x: parent.width; background: blue; }
}
Then the Clip
item for that outer rectangle would still be considered visible.
But the inner Rectangle would not;
I think either behavior is fine. It's just that the documention is a bit strange.
I think what you mean is that if an item has a size of 0x0 it will not be considered visible (because the intersection is empty)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then the Clip item for that outer rectangle would still be considered visible.
But the inner Rectangle would not;
The Clip
item would not be considered visible (the function returns false
). AFAICS the old code did that as per this condition:
if is_clipping && (geometry.width() <= 0.01 as _ || geometry.height() <= 0.01 as _) {
return false;
}
and the new code should do the same by intersection.
Yes, I'm in favor of that - if you're okay with the rest of this API. I'd make that a separate change though. |
b801734
to
02e3ef0
Compare
Going back into draft mode. I'm removing the children bits for now and want to proceed with the matching API that includes descendants but also the other criteria. |
9993ca9
to
fd974dc
Compare
internal/core/item_tree.rs
Outdated
|
||
/// Visit the children of this element and call the visitor to each of them, until the visitor returns [`ControlFlow::Break`]. | ||
/// When the visitor breaks, the function returns the value. If it doesn't break, the function returns None. | ||
fn visit_descendants_internal<R>( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Naming: We often use the _impl
prefix for this kind of helper function, instead of _internal
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume you meant suffix. Changed :)
internal/core/item_tree.rs
Outdated
@@ -312,21 +313,30 @@ impl ItemRc { | |||
r.upgrade()?.parent_item() | |||
} | |||
|
|||
// FIXME: This should be nicer/done elsewhere? | |||
/// Returns true if this item is visible from the root of the item tree. Note that this will return | |||
/// false for `Clip` elements, even if they are "visible". |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A test for that would be nice.
So if you have a
outer := Rectangle {
clip: true;
inner := Rectangle { x: parent.width; background: blue; }
}
Then the Clip
item for that outer rectangle would still be considered visible.
But the inner Rectangle would not;
I think either behavior is fine. It's just that the documention is a bit strange.
I think what you mean is that if an item has a size of 0x0 it will not be considered visible (because the intersection is empty)
assert_eq!(root.match_descendants().match_inherits("Base").find_all().len(), 1); | ||
} | ||
|
||
/* |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is this commented out?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a test that assumes the availability of children traversal, which I removed again. I can remove the commented out test, but I want to bring it back when adding children support.
- ElementHandle::match_descendants() becomes ElementHandle::query_descendants() to emphasize that this creates a query. - Added ElementQuery::from_root() to remove the need to use the ElementRoot trait.
fbeb32b
to
d41b3c2
Compare
No description provided.